patch: DecodeEntity doesn't honor contentType #142

denopink · 2023-10-27T17:03:53Z

The issue lie's in this section:

Lines 45 to 57 in e3db47f

    
           msg, ok := wrpcontext.Get[*wrp.Message](original.Context()) 
        
           if ok { 
        
           	jsonBytes, err := json.Marshal(msg) 
        
           	if err != nil { 
        
           		return nil, err 
        
           	} 
        
           	entity := &Entity{ 
        
           		Message: *msg, 
        
           		Format:  format, 
        
           		Bytes:   jsonBytes, 
        
           	} 
        
           	return entity, nil 
        
           }

Where it sets entity.Bytes to a set of json bytes instead of honoring the content-type format

This eventually becomes a problem when a user sends a msgpack request to talaria and the device receiving that message then expects a msgpack request body... triggering a decoding error on the device's end

Because candlelight eats the request body when it extracts the wrp message:

wrp-go/wrphttp/decoders.go

Lines 111 to 140 in e3db47f

    
           func DecodeRequest(r *http.Request, msg any) (*http.Request, error) { 
        
           	if _, ok := wrpcontext.Get[*wrp.Message](r.Context()); ok { 
        
           		// Context already contains a message, so just return the original request 
        
           		return r, nil 
        
           	} 
        
           	format, err := DetermineFormat(wrp.JSON, r.Header, "Content-Type") 
        
           	if err != nil { 
        
           		return nil, fmt.Errorf("failed to determine format of Content-Type header: %v", err) 
        
           	} 
        
           	var decodedMessage wrp.Message 
        
           	// Try to decode the message using the HTTP Request headers 
        
           	// If this doesn't work, decode the message as Msgpack or JSON format 
        
           	if err = SetMessageFromHeaders(r.Header, &decodedMessage); err != nil { 
        
           		// Msgpack or JSON Format 
        
           		bodyReader := r.Body 
        
           		err = wrp.NewDecoder(bodyReader, format).Decode(&decodedMessage) 
        
           		if err != nil { 
        
           			return nil, fmt.Errorf("failed to decode wrp message: %v", err) 
        
           		} 
        
           	} 
        
           	ctx := wrpcontext.Set(r.Context(), &decodedMessage) 
        
           	// Return a new request with the new context, containing the decoded message 
        
           	return r.WithContext(ctx), nil 
        
           }

the solution appears to be to save the request body in the context along with the wrp message. This allows us to keep the api changes to a minimum without introducing larger patch

This patch will require a one liner update to candlelight and scytale.

maurafortino

looks good to me :)

codecov · 2023-10-27T20:53:32Z

Codecov Report

Merging #142 (8ade81d) into main (b1746ad) will decrease coverage by 0.06%.
Report is 1 commits behind head on main.
The diff coverage is 52.17%.

@@            Coverage Diff             @@
##             main     #142      +/-   ##
==========================================
- Coverage   47.79%   47.74%   -0.06%     
==========================================
  Files          24       24              
  Lines        4352     4344       -8     
==========================================
- Hits         2080     2074       -6     
+ Misses       2087     2081       -6     
- Partials      185      189       +4

Flag	Coverage Δ
unittests	`47.74% <52.17%> (-0.06%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
wrphttp/decoders.go	`67.94% <52.17%> (-0.66%)`	⬇️

denopink added the bug Something isn't working label Oct 27, 2023

denopink self-assigned this Oct 27, 2023

denopink requested review from renaz6, johnabass and maurafortino October 27, 2023 20:18

patch: DecodeEntity doesn't honor contentType

843b6f7

denopink force-pushed the denopink/patch/wrp-DecodeEntity-doesnt-honor-contentType branch from 1181d57 to 843b6f7 Compare October 27, 2023 20:21

This was referenced Oct 27, 2023

chore: update to wrpcontext.GetMessage xmidt-org/scytale#310

Merged

chore: update to wrpcontext.GetMessage xmidt-org/candlelight#155

Merged

maurafortino approved these changes Oct 27, 2023

View reviewed changes

chore: patch decoder test

8ade81d

denopink merged commit 3460c43 into main Oct 30, 2023
13 of 14 checks passed

denopink deleted the denopink/patch/wrp-DecodeEntity-doesnt-honor-contentType branch October 30, 2023 13:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

patch: DecodeEntity doesn't honor contentType #142

patch: DecodeEntity doesn't honor contentType #142

denopink commented Oct 27, 2023 •

edited

Loading

maurafortino left a comment

codecov bot commented Oct 27, 2023 •

edited

Loading

	msg, ok := wrpcontext.Get[*wrp.Message](original.Context())
	if ok {
	jsonBytes, err := json.Marshal(msg)
	if err != nil {
	return nil, err
	}
	entity := &Entity{
	Message: *msg,
	Format: format,
	Bytes: jsonBytes,
	}
	return entity, nil
	}

	func DecodeRequest(r http.Request, msg any) (http.Request, error) {

	if _, ok := wrpcontext.Get[*wrp.Message](r.Context()); ok {
	// Context already contains a message, so just return the original request
	return r, nil
	}

	format, err := DetermineFormat(wrp.JSON, r.Header, "Content-Type")
	if err != nil {
	return nil, fmt.Errorf("failed to determine format of Content-Type header: %v", err)
	}

	var decodedMessage wrp.Message

	// Try to decode the message using the HTTP Request headers
	// If this doesn't work, decode the message as Msgpack or JSON format
	if err = SetMessageFromHeaders(r.Header, &decodedMessage); err != nil {
	// Msgpack or JSON Format
	bodyReader := r.Body
	err = wrp.NewDecoder(bodyReader, format).Decode(&decodedMessage)
	if err != nil {
	return nil, fmt.Errorf("failed to decode wrp message: %v", err)
	}
	}

	ctx := wrpcontext.Set(r.Context(), &decodedMessage)

	// Return a new request with the new context, containing the decoded message
	return r.WithContext(ctx), nil
	}

patch: DecodeEntity doesn't honor contentType #142

patch: DecodeEntity doesn't honor contentType #142

Conversation

denopink commented Oct 27, 2023 • edited Loading

maurafortino left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 27, 2023 • edited Loading

Codecov Report

denopink commented Oct 27, 2023 •

edited

Loading

codecov bot commented Oct 27, 2023 •

edited

Loading